Skip to content

Fix colocated source maps#763

Open
wagenet wants to merge 1 commit into
ember-cli:masterfrom
wagenet:fix-colocated-template-sourcemaps
Open

Fix colocated source maps#763
wagenet wants to merge 1 commit into
ember-cli:masterfrom
wagenet:fix-colocated-template-sourcemaps

Conversation

@wagenet

@wagenet wagenet commented Jan 10, 2023

Copy link
Copy Markdown

Resolves #558.

@dagroe

dagroe commented Jan 24, 2023

Copy link
Copy Markdown

Thanks for you MR! I'd really like to see this issue fixed for easier debugging.

I tried it on our app, but unfortunately don't get it to work properly.

Without this fix we get sourcemaps for our application-name.js that point to our components' .ts files, although with the offset in line numbers.

With this fix we get sourcemaps for our application-name.js that point to the transpiled .js files. The mapping to .ts files is missing.

When looking in to it, it seems your changes correctly apply the template prefix and add the sourcemap to our .ts files. But then those sourcemaps seem to be incompatible with the babel transpilation.

@wagenet

wagenet commented Jan 25, 2023

Copy link
Copy Markdown
Author

@dagroe Yeah, this is the problem I have. I expected this change to fix stuff, but it doesn't seem to have. I have to dig in more. These source maps should be ingestible by other tools, so I'm not sure what's going on there. Maybe we have to change a downstream setting.

@dagroe

dagroe commented Jan 25, 2023

Copy link
Copy Markdown

@wagenet Glad I am not the only one with this problem 😉

I did a bit more digging.

We do not get sourcemaps since the generated sourcemaps fail validation. When sourcemaps are built by broccoli-concat it uses fast-sourcemap-concat which in turn runs validation via sourcemap-validator. Here validation simply fails because our sourcemaps are missing the correct column positions.

Luckily we can fix that by passing hires: true to jsContentsMagic.generateMap.

Second issue is that the sourcemaps do not have the correct paths to sources. E.g. a component in components/list/item.ts will only have item.ts in sources. What works for me was the following:

Store the correct extension:

let ext;
if (this.inputHasFile(basePath + '.js')) {
  backingClassPath += '.js';
  ext = '.js';
  hasBackingClass = true;
} else if (this.inputHasFile(basePath + '.ts')) {
  backingClassPath += '.ts';
  ext = '.ts';
  hasBackingClass = true;
} else if (this.inputHasFile(basePath + '.coffee')) {
  backingClassPath += '.coffee';
  ext = '.coffee';
  hasBackingClass = true;
} else {
  backingClassPath += '.js';
  ext = '.js';
  hasBackingClass = false;
}

then use it so set the correct file and source:

let file = filePathParts.name + ext;
let jsContentsMap = jsContentsMagic.generateMap({
  source: backingClassPath,
  file,
  includeContent: true,
  hires: true
});

This seem to generate correct sourcemaps.

@wagenet

wagenet commented Jan 26, 2023

Copy link
Copy Markdown
Author

@dagroe Great! I'm so glad you were able to look into this. Would you prefer to make your own PR or should I make the fixes in my PR when I'm able?

@dagroe

dagroe commented Jan 26, 2023

Copy link
Copy Markdown

@wagenet Thanks for merging my changes :)

Maybe we can get someone else to test this as well. I know it's been some time but maybe @beddueimpossibile or @bartocc are still interested?

I'll do some more testing. So far source maps look fine when debugging in chrome or firefox but I haven't been able to make it work correctly in pycharm yet.

@wagenet wagenet force-pushed the fix-colocated-template-sourcemaps branch from f59355d to 3407a9b Compare January 26, 2023 21:24
@bartocc

bartocc commented Jan 27, 2023

Copy link
Copy Markdown

Thx for flagging me! I am still very interested and I would love to see this fixed! Gonna try this PR right now!

@bartocc

bartocc commented Jan 27, 2023

Copy link
Copy Markdown

I've done some testing on our app and with VSCode.

I've changed package.json with

- "ember-cli-htmlbars": "^6.1.0",
+ "ember-cli-htmlbars": "wagenet/ember-cli-htmlbars#fix-colocated-template-sourcemaps",

setup VSCode launch setting with:

"sourceMapPathOverrides": {
  "my-app/*": "${workspaceFolder}/app/*"
}

Unfortunately, I do not see any improvements.
Here are my results:

classic structure + JS component class: breakpoint binds

app/components/foo-bar.js
app/templates/components/foo-bar.hbs

classic structure + TS component class: breakpoint binds

app/components/foo-bar.ts
app/templates/components/foo-bar.hbs

flat structure (ie co-located template) + JS component class: breakpoint DOES NOT BIND

app/components/foo-bar.js
app/components/foo-bar.hbs

flat structure (ie co-located template) + TS component class: breakpoint DOES NOT BIND

app/components/foo-bar.ts
app/components/foo-bar.hbs

@dagroe

dagroe commented Jan 27, 2023

Copy link
Copy Markdown

@bartocc I tested it using a new dummy project, which I put here: https://github.com/dagroe/test-ember-sourcemaps

The breakpoint in components/test/foo-bar.ts was hit using vscode with this launch.json:

{
    // Use IntelliSense to learn about possible attributes.
    // Hover to view descriptions of existing attributes.
    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
    "version": "0.2.0",
    "configurations": [
        {
            "type": "chrome",
            "request": "launch",
            "name": "Launch Chrome against localhost",
            "url": "http://localhost:4300",
            "webRoot": "${workspaceFolder}",
            "sourceMapPathOverrides": {
                "html-bars-test/*": "${workspaceRoot}/app/*"
            },
        }
    ]
}

Still trying to figure it out for pycharm, but the sourceMapPathOverrides seems to be the critical part in vscode.

@bartocc

bartocc commented Jan 27, 2023

Copy link
Copy Markdown

Thx for setting up this demo project @dagroe. I need to check our own project to see why the breakpoints do not bind for co-located components…

But if I am correct, everything works fine when using wagenet/ember-cli-htmlbars#fix-colocated-template-sourcemaps, doesn't it?

PS: In the mean time, I've prepared a small PR dagroe/test-ember-sourcemaps#2 to bundle the launch config you used in your previous post to help others test it

@dagroe

dagroe commented Jan 27, 2023

Copy link
Copy Markdown

Thanks for your PR :)

Works fine for me when using wagenet/ember-cli-htmlbars#fix-colocated-template-sourcemaps. Source maps look correct in browser debug (firefox and chrome) and work as expected in VSCode. Also making changes, reloading etc. seems to work fine.

@wagenet wagenet marked this pull request as ready for review January 27, 2023 19:01
@dagroe

dagroe commented Mar 22, 2023

Copy link
Copy Markdown

@chriskrycho @rwjblue @dfreeman sorry for the ping but I am not sure who to ask for a review of this. Could anyone of you do it or assign someone? Thanks!

@wagenet

wagenet commented Mar 22, 2023

Copy link
Copy Markdown
Author

I just tried this is another app of my own and it appeared to drastically slow down the build. I'm not sure if the speed issue is actually related or if it's just a red herring. I'd like to try to get that figured out soon.

@dagroe

dagroe commented Mar 30, 2023

Copy link
Copy Markdown

That would be a big drawback 😞 , but it seems to make sense that generating sourcemaps with hires: true for all colocated components will slow down the build.

I was thinking that there might be a quicker way to generate the sourcemaps since source and generated files should be identical besides the two added lines for the template. I need to find some time to look into whether it makes sense to build the sourcemaps ourselves.

@dagroe

dagroe commented Mar 30, 2023

Copy link
Copy Markdown

Seems generating the source map is straight forward. I added it here: https://github.com/dagroe/ember-cli-htmlbars/tree/try-custom-sourcemap-generation @wagenet can you give this a try to see if it still works and improves build times in your project? 😄

@wagenet wagenet force-pushed the fix-colocated-template-sourcemaps branch from 4fc947c to 1ec1645 Compare March 30, 2023 21:06
@dagroe

dagroe commented Jul 10, 2023

Copy link
Copy Markdown

@wagenet Seems source maps are fixed in 6.2.0, at least for me they are working again without this PR.

@locks locks self-assigned this Jul 14, 2023
@wagenet wagenet force-pushed the fix-colocated-template-sourcemaps branch 2 times, most recently from daec2a9 to 253bd70 Compare November 15, 2023 22:30
@wagenet

wagenet commented Nov 15, 2023

Copy link
Copy Markdown
Author

I took another pass at this and figured out a couple things. First, I fixed the paths so that source maps should fully work. (Note that because of unrelated issues in the build process, you may need to set them as "inline" in your babel config.)

As far as concerns about the build being slowed down, this seems to be due to a bad interaction with ember-template-imports v3 which parses all .js and .ts files. Somehow adding the source maps can end up being very bad for this.

Comment thread pnpm-lock.yaml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming didn't mean to introduce this

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops!

@dmyoung9

dmyoung9 commented Sep 15, 2024

Copy link
Copy Markdown

@wagenet Seems we are not able to install your fork directly via npm, due to this line.

I was able to get it to work by cloning locally, removing that line, and installing it from my local filesystem, but this is not ideal.

Would it be possible to remedy this at the source?

EDIT: I can install it using pnpm, but this seems to cause issues with my other dependencies.

@dmyoung9

Copy link
Copy Markdown

I took another pass at this and figured out a couple things. First, I fixed the paths so that source maps should fully work. (Note that because of unrelated issues in the build process, you may need to set them as "inline" in your babel config.)

As far as concerns about the build being slowed down, this seems to be due to a bad interaction with ember-template-imports v3 which parses all .js and .ts files. Somehow adding the source maps can end up being very bad for this.

Unfortunately, using sourceMaps: 'inline' breaks debugging in Chrome DevTools... If I use just your fork, with no other modifications, debugging works in Chrome DevTools and code coverage is accurate, but I still haven't been able to get VS Code breakpoints to bind properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants